Rework KeyPathArray filters for notifications in the C-API. #7087
Conversation
jedelbo
force-pushed
the
cm/rework-c-keypath-filtering
branch
from
e4b7aca to
046616f
Compare
|
@cmelchior should this be validated in Kotlin before merged? |
Pull Request Test Coverage Report for Build github_pull_request_285448
💛 - Coveralls |
|
I'm finishing up the tests on Kotlin. So far, this seems to work, but I had to add support for backlinks pr. #7131. It feels a bit hacky though and like something is missing from the ObjectStore Schema APIs |
jedelbo
force-pushed
the
cm/rework-c-keypath-filtering
branch
from
9cfa503 to
be45032
Compare
|
I tries to update the col_key for computed properties during realm-core/src/realm/object-store/object_store.cpp Lines 945 to 951 in be45032 @tgoyne I am not sure if this is sufficient. |
|
I don't see any obvious reason that won't work. |
|
Not 100% sure where the error is, but this doesn't work in Kotlin. Keypaths for normal Results work fine, but are broken for backlinks....Maybe the C-API creates the schema in a different way that doesn't end up hitting that codepath? 🤔 |
|
Not 100% sure what is going on, but it looks like opening the same Realm multiple times will result in |
jedelbo
force-pushed
the
cm/rework-c-keypath-filtering
branch
from
fce8171 to
c0b6c00
Compare
|
I confirmed that this now works in Kotlin, so moving the PR to review. Not 100% sure who should review it though, since @jedelbo did most of the rewrite of the implementation? |
| return ret; | ||
| } | ||
|
|
||
| static KeyPathArray create_key_path_array(Group& g, const ObjectSchema& object_schema, const Schema& schema, |
There was a problem hiding this comment.
Wouldn't there be value in having this method available to SDKs that do not use the C-API? I guess it is already available since it is just a static helper method, but maybe the location is a bit wierd.
There was a problem hiding this comment.
A static function is actually private within the compilation unit. We might consider making it public somewhere. At least to the binding generator.
src/realm.h
Outdated
| */ | ||
| RLM_API realm_key_path_array_t* realm_create_key_path_array(const realm_t* realm, | ||
| const realm_class_key_t object_class_key, | ||
| int user_key_paths_count, const char** user_key_paths); |
There was a problem hiding this comment.
I guess user_key_paths_count cannot be negative.
| @@ -361,6 +361,14 @@ constexpr inline size_t round_down(size_t p, size_t align) | |||
| return r & (~(align - 1)); | |||
| } | |||
|
|
|||
| // return pointer to found character or to terminating NUL | |||
| static inline const char* find_chr(const char* p, char c) | |||
There was a problem hiding this comment.
Why do we need this utility? It seems to me that it is just a char *strchr(const char *string, int c);
There was a problem hiding this comment.
strchr will return nullptr if not found. We need it to return a pointer to terminating NUL character. GCC has a function called strchrnul, but that is not generally available.
There was a problem hiding this comment.
OK, I think it does not make a huge difference if we call strchr() and we check for nullptr in order to return a null terminated string or loop over like we do.
This PR replaces the current way of using the KeyPathArray API in the C-API with an approach that moves more work to C++. So instead of looking up ClassKey/ColumnKeys in the SDK, this is instead done in a helper function in Core.
This means that SDKs just need to pass forward user input to Core and it will create the correct data structure.
Since no SDKs have exposed this through the C-API yet, this should be safe to break.
TODO: